Skip to content

Conversation

@juan518munoz
Copy link
Collaborator

@juan518munoz juan518munoz commented Dec 18, 2025

Replace logic for adding authenticated input notes in a Transaction. User no longer provides the NoteIDs of the authenticated note he wants to use, but rather send the Note directly (being authenticated or not), and let the Client decide if the note passed is verified.

Note: most test cases have a new step when building transaction requests with authenticated notes. Instead of submitting the transaction with the noteID, they retrieve the associated note with it from the Client, and submit the tx with that info.

Closes #1112

@juan518munoz juan518munoz changed the title refactor: input not authentication deduced by client refactor: input note authentication deduced by client Dec 19, 2025
@juan518munoz juan518munoz marked this pull request as ready for review December 19, 2025 14:27
Comment on lines -1947 to -1968
// Check that adding an authenticated note that is not tracked by the client will return an
// error
let missing_authenticated_note_tx_request = TransactionRequestBuilder::new()
.build_consume_notes(vec![NoteId::from_raw(EMPTY_WORD)])
.unwrap();
let error =
Box::pin(client.submit_new_transaction(wallet.id(), missing_authenticated_note_tx_request))
.await
.unwrap_err();

assert!(matches!(
error,
ClientError::TransactionRequestError(
TransactionRequestError::MissingAuthenticatedInputNote(_)
)
));
Copy link
Collaborator Author

@juan518munoz juan518munoz Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer errors if the Note passed is not authenticated, It will just not get marked as such (by the Client).

Comment on lines 220 to 230
authenticated_note_records.retain(|note| {
matches!(
note.state(),
InputNoteState::Committed(_)
| InputNoteState::ConsumedAuthenticatedLocal(_)
| InputNoteState::ConsumedExternal(_)
)
});

let authenticated_note_ids =
authenticated_note_records.iter().map(InputNoteRecord::id).collect::<Vec<_>>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this two steps can probably be done altogether

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep them apart as both authenticated_note_records and authenticated_note_ids are used separately afterwards.

Comment on lines 605 to 606
.filter_map(|note| {
if transaction_request.input_notes().iter().any(|n| n.id() == note.id()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are we excluding unauthenticated ones here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we weren't, good catch, addressed in a9e5113

Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, but I think we should tweak some things before merging. Specifically, unless I'm missing something, it looks like we are adding all notes as InputNote::Unauthenticated and we should probably make the distinction before passing the notes to the VM before executing. I left a comment about this, and some other nits as well.

One thing that would be pending for this PR is the last bullet point in the issue, but maybe that's something to tackle in a different issue/PR

/// Optional arguments of the Notes to be consumed by the transaction. This
/// includes both authenticated and unauthenticated notes.
input_notes: Vec<(NoteId, Option<NoteArgs>)>,
input_notes_args: Vec<(NoteId, Option<NoteArgs>)>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the comment above, should this just be input_notes_args: Vec<(Note, Option<NoteArgs>)>? Instead of having the extra level of indirection.

Copy link
Collaborator Author

@juan518munoz juan518munoz Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra level of indirection

Thought about that, I believe the best approach is to merge input_notes and input_notes_args into a single field (input_notes). Should we go with it?

Comment on lines +64 to 65
/// Notes to be consumed by the transaction.
/// includes both authenticated and unauthenticated notes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should clarify here (and in TransactionRequestBuilder) this new behavior about the client consuming a note as unauthenticated when it's not present in the store and as authenticated when it is present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 8db0e04

/// specified authenticated notes need to be provided, otherwise an error will be returned.
/// The transaction input notes will include both authenticated and unauthenticated notes in the
/// order they were provided in the transaction request.
pub(crate) fn build_input_notes(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this function's doc comments

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I think this can be a private function

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I think this can be a private function

The function is called from impl Client<AUTH> if I'm not mistaken, so it needs to be at least pub(crate).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this function's doc comments

I don't see which part of the docs needs changing, it still only takes authenticated notes as InputNoteRecords.

Comment on lines 216 to 218
let mut authenticated_note_records = self
.store
.get_input_notes(NoteFilter::List(authenticated_input_note_ids))
.get_input_notes(NoteFilter::List(transaction_request.get_input_note_ids()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to retrieve the full NoteRecord objects here, right? Let's add a TODO (or if you prefer, an issue) to see whether we should add a specific store method to return authenticated note IDs from a set of notes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you point out, it's quite redundant to retrieve all notes, as we already pass them as arguments. On the other hand, we need to check that the notes are not consumed and also filter as authenticated, AFAIK this can only be done through the InputNoteRecord struct.

Am I missing something?

Comment on lines 231 to 239
// Add unauthenticated input notes to the input notes map.
for unauthenticated_input_notes in &self.unauthenticated_input_notes {
for unauthenticated_input_notes in &self.input_notes {
input_notes.insert(
unauthenticated_input_notes.id(),
InputNote::Unauthenticated {
note: unauthenticated_input_notes.clone(),
},
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not add all authenticated notes as well? If so, we should make it so that authenticated notes are consumed exclusively as that instead of added as InputNote::Unauthenticated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, addressed in 8db0e04

Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left a couple of comments but it's good to merge already

@igamigo
Copy link
Collaborator

igamigo commented Jan 13, 2026

Pushed a commit that addresses my above comments

@igamigo igamigo merged commit 41807e5 into next Jan 13, 2026
29 checks passed
@igamigo igamigo deleted the jmunoz-simplify-transaction-builder-note-inputs branch January 13, 2026 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make client decide if input notes are authenticated or not

5 participants